fix(herdr-core): mark a finish seen again when a person comes back to its pane - #2779

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: devswha/ovis
• Branch: fix/issue-2690
• File: .coderabbit.yaml
• Commit: 84171f3
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: e661e346-27c8-4844-98c5-2301a95a160d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

Merge Risk: Moderate · previous review 6884d

Returning to a previously shown finish can leave it unread, contrary to the intended behavior. A concurrent turn change can also mark an unseen finish read. Resolve these issues before merging.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Linked Issues check		[#2690] Objective A: returning to a pane after another client marked its finish unread must raise herdr.seen to the turn again. The server side works. SeenMarker.write (server/herdr-core/seen-label.ts…	In src/lib/sidebarActivity.tsx, send presented "new" for a finish already shared when the pane returns to the front after it left the front since the last send. Do not limit this to previous.presented === "held". Keep the pane-in-front chec…
Docstring Coverage		Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: marking a finish seen again when a person returns to its pane.
Description check		The description includes complete Change and Validation sections. It explains the behavior, API contract, implementation scope, linked issue, passed checks, and skipped validation.
Out of Scope Changes check		All changed files relate to #2690 and the presented-field contract. Changes cover: shared/seen.ts (SeenPresentation, seenPresentationOf); src/lib/api.ts (optional presented in shareSeen); src/lib/side…

Full details: Out of Scope Changes check

Explanation

All changed files relate to #2690 and the presented-field contract. Changes cover: shared/seen.ts (SeenPresentation, seenPresentationOf); src/lib/api.ts (optional presented in shareSeen); src/lib/sidebarActivity.tsx and its hook test; server/seen.ts and server/seen.test.ts (presentation notifications); server/index.ts (again flag on markSeen); server/herdr-core/seen-label.ts and adapter.test.ts (again write path, written-turn handling); docs/guide.md; and the changelog entry. The tests and docs support the same contract. No unrelated change was found in the change summaries. This result relies on the per-file summaries and the head source of seen-label.ts and sidebarActivity.tsx. The full diff was not read. The hook test file was truncated, so its assertions were taken from the test summary.

Full details: Linked Issues check

Explanation

[#2690] Objective A: returning to a pane after another client marked its finish unread must raise herdr.seen to the turn again. The server side works. SeenMarker.write (server/herdr-core/seen-label.ts) writes when again is true and herdr.seen < turn. The browser does not send presented "new" in the common case. In src/lib/sidebarActivity.tsx, a finish already shared is re-sent only when shownAgain holds (previous.presented === "held" && presented === "new"). The other send conditions need a higher finish id or an answer upgrade. So a finish first shared as "new" (the pane was opened after the finish arrived) is not re-sent when the pane returns. The hook test summary confirms this: "later returns after it has been reported as shown do not trigger another notification." This is the issue's Steps 1-3 (read, switch away, mark unread elsewhere, return). The label stays unread for the other client. Objective B: a turn another client marked seen first counts as written. SeenMarker.write records it as written when seen >= turn, so a later unread on screen is not overwritten. This objective is met. Reconnect is not a new look, which is met by frontKey. Tests cover the held-to-new return path, but they assert no re-notify for a new-then-return finish, which codifies the gap.

Resolution

In src/lib/sidebarActivity.tsx, send presented "new" for a finish already shared when the pane returns to the front after it left the front since the last send. Do not limit this to previous.presented === "held". Keep the pane-in-front check so a pane that stays in front sends nothing more. Keep reconnect as no new look. Update the sidebarActivity.hook.test.ts case that asserts no notification after a finish was reported as shown, so that leaving and returning sends "new". Add a server or adapter test that a return after a "new" first read raises herdr.seen again when another client lowered it.

Full details: Docstring Coverage

Explanation

Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 9 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Review findings and how they were resolved

This PR changes an HTTP contract (POST /api/seen). CodeRabbit reviewed the first head (6884d7e); omo and Codex reviewed both heads. Findings and resolutions:

omo (dori review, head 6884d7e): CLEAR, no findings.

Codex (gpt-6.1-terra, xhigh, adversarial, head 6884d7e):

1. HIGH - readiness (paneInFront: content marker, overlays) or a reconnect dropping and coming back counted as a new presentation, so an unread told meanwhile could be overwritten. Fixed: a pane is brought into view only when it becomes the selected pane on a visible page; readiness only gates when the look is told. Test: overlay and reconnect steps in the new hook test now expect held.
2. HIGH - a turn read elsewhere while the pane sat idle in front here was not remembered (Ovis is told of no look at an idle pane), so a later unread was overwritten by the held look at the new finish. Fixed: SeenMarker.observe learns each terminal's turn and herdr.seen from the adapter's events subscription (observeSeen); a turn some client marked seen counts as written. Tests: t5 in the marker test, the observer calls in the events test.
3. HIGH - a retried new could replay an old look after an unread told since. Fixed: a send that is put off (auth/outage backoff) or retried is told as held. Test: the backoff tests now expect the delivered body with presented: "held".
4. MEDIUM - coming back to a finish already told as new sends nothing. Not changed: a second look at the same finish only matters when the first label write failed (logged, not retried, a limit of feat(herdr-core): share read finishes with the Herdr app through herdr.seen #2687); a read and unread elsewhere make a new finish, which is sent. Sending on every return would add a POST per pane switch.
5. MEDIUM - an answer upgrade could replace a pending new with held. Resolved by 3: a pending send is held anyway.
6. MEDIUM - queued marker work fetches the terminal's turn when it runs, so it can mark a turn that ended after the look. Pre-existing (the same race exists for every mark since feat(herdr-core): share read finishes with the Herdr app through herdr.seen #2687, not introduced here). The window is one terminal.get round trip while a turn ends, so it is noted here rather than filed.

Codex round 2 (head d7d04a3, after the fixes above): no HIGH; three MEDIUM, left as they are under the lane's one-extra-round rule:

• Opening and closing the navigation drawer on a phone (presentedPaneId goes null) counts as bringing the pane into view again, so a finish told as held is told as new. Closing the drawer does show the pane again, so this is close to a real look; tracking the selected pane apart from the presented one would make it held.
• A return to the pane while its held share is still in flight (or waiting to retry) is not told as new: told is set only once the request answers. Narrow window; the next finish or return is told.
• An answer upgrade of the same finish replaces its share and forgets that it was told as new, so a later return tells new again. That return is a real look, so the harm is only one extra write.

CodeRabbit (head 6884d7e):

• Major, sidebarActivity.hook.test.ts (re-send new on every return): same as Codex 4 above. When another client marks a finish unread, the server's finish counter makes it a new finish, which the browser sends; so "herdr.seen stays as another client set it" does not happen through that path. Kept as is, to avoid a POST per pane switch.
• Minor, server/index.ts (bind the write to the presented turn, pane.revision): same as Codex 6, pre-existing since feat(herdr-core): share read finishes with the Herdr app through herdr.seen #2687. A good small follow-up.

Validation: bun run check fast passed on d7d04a3 (typecheck, build, unit tests).

omo round 2 (head d7d04a3): BLOCK, fixed

• MEDIUM (reproduced): a finish that arrived while an overlay (Settings, the palette, the drawer...) covered the pane was told as held when it closed, so after observe() it stayed unread in the Herdr app although it was the person's first look. Fixed: App.tsx passes panePresence (front, loading or covered) in place of paneInFront; closing an overlay brings the pane into view again (new), while a reconnect or content still loading does not. Test: the overlay step in the hook test now expects new and fails against d7d04a3. This also settles Codex round 2's drawer note: closing the drawer is a look.
• MEDIUM (not reproduced): a return while a held share is in flight is not told as new. Same as Codex round 2's second note; narrow window, left as is.
• LOW: a cached older bundle sends no presented; it behaves as held until the page reloads.

The branch also merges main (conflict in server/index.ts imports only).

Deploying ovis with    Cloudflare Pages

View logs

Follow-up for Codex round 2's reproduced MEDIUMs (a return while a held share is in flight; an answer upgrade forgetting a new): #2781

Codex round 2's drawer note is now intended behaviour: closing the drawer or an overlay counts as a look (panePresence, head 84171f3). The unreproduced notes (omo's in-flight duplicate of the first item, the pre-existing terminal.get turn race, an older cached bundle) stay in the comments above only.

Merge note: merged on the omo review (CLEAR at this head), Codex Terra (no HIGH; reproduced findings fixed, the rest in the PR comments or follow-up issues) and green required checks (Integration and browser passed). Jobs still shown as in progress or queued were stuck by GitHub's Actions incident (steps already succeeded) or are non-required macOS jobs waiting for runners. For contract changes, review findings and resolutions are in the PR comments.